Migrate to connect-es #354

edaniels · 2024-09-17T22:03:18Z

Hey all, we talked about doing this migration in #frontend-friends. I figured it would be best to review this code in this repo first, get it merged, and then move it over to the TS SDK. If we somehow need some patches in the meantime, we can just branch off (or worst case revert).

Connect supports pretty much everything we need (node, react native, browser, and more). We can continue the model of having a global.VIAM for setting transports and other platform specific needs.

Before:

✓ 51 modules transformed.
dist/main.js  663.78 kB │ gzip: 87.59 kB
✓ built in 678ms

After:

✓ 133 modules transformed.
dist/main.js  130.79 kB │ gzip: 31.52 kB
✓ built in 356ms

rpc/examples/echo/node-client/src/index.ts

proto/rpc/examples/fileupload/v1/fileupload.proto

buf.lock

rpc/examples/echo/frontend/src/index.ts

rpc/js/tsconfig.json

maximpertsov · 2024-09-18T12:14:10Z

rpc/js/src/ClientChannel.ts

-    const id = stream.getId();
-    const activeStream = this.streams[id];
+    const { id } = stream;
+    const activeStream = this.streams[Number(id)];


Suggested change

const activeStream = this.streams[Number(id)];

const activeStream = this.streams[id];

Not sure how likely we are to hit a stream count where this would matter, but maybe we should just use BigInt throughout? My understanding is that coercing between BigInt and Number could lead to precision loss per MDN docs:

Be careful coercing values back and forth, however, as the precision of a BigInt value may be lost when it is coerced to a Number value.

The proto changed this to a bigint so we could also just change the property on the map?

change the property on the map

yep let's do that

so bigint cannot be used as an index which makes sense, so how about the record key be a string and we go bigint->string on demand? it has a cost but I don't think we're in that realm of performance needs just yet

that works too

maximpertsov

This is awesome!

I have one minor comment related to BigInt/Number coercion. Otherwise LGTM.

mcous

Very excited about this work, it's a huge piece of nagging tech debt off our shoulders!

I've done an initial review pass to highlight some areas of confusion for me that I'd like to focus on in our in-person review tomorrow. Also tried to highlight areas where I think extra cleanup, testing, and/or comments would be helpful, either in this PR or in the future, to ensure we're going to be able to maintain this moving forward.

If it can be practically done for this PR, we've been doing a lot of work in Fleet and Viz to use PR/commit stacking to break up large bits of work into smaller chunks. If you're down, I think splitting the updates to examples into their own PRs might make this easier to review, understand, and land. I also wonder if there's a way we could really minimize the examples changes to get a better sense of just how much of a "breaking" change this is - as opposed to opportunistic cleanup of the examples. (I think example cleanup is valuable but could happen in follow-up PRs by other contributors to gain familiarity.)

rpc/js/src/dial.ts

rpc/examples/echo/frontend/src/index.ts

rpc/js/src/dial.ts

rpc/js/tsconfig.json

rpc/js/vite.config.ts

rpc/js/src/dial.ts

rpc/js/src/errors.ts

edaniels · 2024-09-20T21:17:25Z

did a bunch of cleaning. next step is a signaling refactor and a comments pass if needed. then ready for re-review. @mcous @ethanlook feel free to take a look at the ClientStream/UnaryClientStream/StreamClientStream refactor. I think it's a lot easier to read now and it minimizes the complexity of the new Promise logic as minimally as I could.

edaniels · 2024-09-22T06:17:51Z

Also made some changes to the buf yaml to reduce how many files this PR changes. There was an unneeded version upgrade.

mcous

Still working through this, but here's a quick batch of housekeeping comments just to keep things moving

rpc/js/src/BaseChannel.ts

rpc/js/src/ClientStream.ts

rpc/js/src/ClientChannel.ts

rpc/js/src/main.ts

rpc/examples/echo/node-client/src/index.ts

rpc/js/tsconfig.json

mcous

I think this is a great state for a testing release! Appreciate all your work on this

rpc/js/src/dial.ts

rpc/js/src/signaling-exchange.ts

ethanlookpotts

Looks great, thanks for the lint cleanup as well! And thank you again for walking us through this code.

rpc/js/src/dial.ts

rpc/js/src/SignalingExchange.ts

edaniels requested review from mcous, ethanlookpotts and maximpertsov September 17, 2024 22:03

viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Sep 17, 2024